Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow gRPC calls with larger query responses #3335

Merged
merged 3 commits into from
May 16, 2023

Conversation

ljoss17
Copy link
Contributor

@ljoss17 ljoss17 commented May 15, 2023

Closes: #3334

Description

This PR has the following updates:

  • Adds a new undocumented configuration max_grpc_decoding_size which can be used to specify a new decoding message size upper limit
  • Updates the gRPC clients decoding message size limit with max_decoding_message_size(). If the the max_grpc_decoding_size is not configured, sets the limit to 32 MiB.
  • Adds the name of the failed query to the GrpcStatus error output message

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@ljoss17 ljoss17 added I: rpc Internal: related to (g)RPC I: configuration Internal: related to Hermes configuration labels May 15, 2023
@@ -472,6 +473,8 @@ pub struct ChainConfig {
pub max_msg_num: MaxMsgNum,
#[serde(default)]
pub max_tx_size: MaxTxSize,
#[serde(default)]
pub max_grpc_decoding_size: Option<Byte>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's set the default value here so that we avoid the if/else at each use site.

@@ -120,6 +122,8 @@ pub mod types;
pub mod version;
pub mod wait;

pub const DEFAULT_GRPC_MAX_MESSAGE_LENGTH: u64 = n_mib_bytes(32);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this and use a default straight in the config struct to avoid the if/else's below

{ status: GrpcStatus }
|e| { format!("gRPC call failed with status: {0}", e.status) },
{ status: GrpcStatus, query: String }
|e| { format!("gRPC call `{}` failed with status: {1}", e.query, e.status) },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

@romac romac merged commit b9f1cf1 into master May 16, 2023
@romac romac deleted the luca_joss/grpc-query-length-limit branch May 16, 2023 08:55
romac pushed a commit that referenced this pull request May 16, 2023
* Adds a new undocumented configuration `max_grpc_decoding_size` which can be used to specify a new decoding message size upper limit
* Updates the gRPC clients decoding message size limit with `max_decoding_message_size()`. If the the `max_grpc_decoding_size` is not configured, sets the limit to `32 MiB`.
* Adds the name of the failed query to the `GrpcStatus` error output message

---

* Increase gRPC max decoding message size using default value or configured value

* Remove const value for max_grpc_decoding_size and use default instead
pratikbin pushed a commit to pratikbin/hermes that referenced this pull request May 24, 2023
* Adds a new undocumented configuration `max_grpc_decoding_size` which can be used to specify a new decoding message size upper limit
* Updates the gRPC clients decoding message size limit with `max_decoding_message_size()`. If the the `max_grpc_decoding_size` is not configured, sets the limit to `32 MiB`.
* Adds the name of the failed query to the `GrpcStatus` error output message

---

* Increase gRPC max decoding message size using default value or configured value

* Remove const value for max_grpc_decoding_size and use default instead
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: configuration Internal: related to Hermes configuration I: rpc Internal: related to (g)RPC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow gRPC calls with query responses larger than 4MiB
2 participants